Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use proxy-protocol to pass through source IP to nginx #675

Merged
merged 1 commit into from
May 10, 2017

Conversation

dpratt
Copy link
Contributor

@dpratt dpratt commented Apr 28, 2017

This is a PR that helps address #672.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 28, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 46.898% when pulling 4465f3a on dpratt:master into f5d27bf on kubernetes:master.

@dpratt
Copy link
Contributor Author

dpratt commented Apr 29, 2017

CLA signed.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 29, 2017
@aledbf aledbf self-assigned this Apr 29, 2017
@dpratt
Copy link
Contributor Author

dpratt commented Apr 29, 2017

Full disclosure: This PR represents essentially my very first attempts at writing golang in anger, so please do not go easy on me if it sucks - I want to know if things could be done better/more idiomatic.

//Write out the proxy-protocol header
localAddr := conn.LocalAddr().(*net.TCPAddr)
remoteAddr := conn.RemoteAddr().(*net.TCPAddr)
protocol := "UNKNOWN"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be

		protocol := "TCP4"
		if remoteAddr.IP.To16() != nil {
			protocol = "TCP6"
		}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that - I wasn't sure if one of To4() and To16() != nil is guaranteed with the golang net stdlib - the proxy-protocol spec says that if it's not either IPv4 or IPv6 the protocol header should read 'UNKNOWN'.

@dpratt
Copy link
Contributor Author

dpratt commented May 1, 2017

Do you want me to push a commit that makes that change?

@johntdyer
Copy link

Bump

@dpratt
Copy link
Contributor Author

dpratt commented May 10, 2017

Just checked up on this and saw that there's been a merge conflict since I submitted. I'll push a resolution today.

@dpratt
Copy link
Contributor Author

dpratt commented May 10, 2017

Conflict resolved and pushed.

We're currently using a custom build of ingress for our internal controller - any chance of this being merged anytime soon?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 46.983% when pulling e77c4fb71d184f4a9e66cb2ac8fc9c76b71fd640 on dpratt:master into 317f222 on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented May 10, 2017

@dpratt please check your PR contains a not related commit. Please fix this and squash.

@dpratt
Copy link
Contributor Author

dpratt commented May 10, 2017

@aledbf Not really sure what you mean - there are two commits, one that fixes the issue, and one that merges master in to fix the merge conflict.

I'd be happy to rebase and resubmit, if that's what you're asking for.

@dpratt
Copy link
Contributor Author

dpratt commented May 10, 2017

I've rebased.

@aledbf
Copy link
Member

aledbf commented May 10, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2017
@aledbf
Copy link
Member

aledbf commented May 10, 2017

@dpratt thanks!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 46.983% when pulling d56d8b7 on dpratt:master into 317f222 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 46.953% when pulling d56d8b7 on dpratt:master into 317f222 on kubernetes:master.

@aledbf aledbf merged commit c1cf8ff into kubernetes:master May 10, 2017
@zllovesuki
Copy link

This still gives 127.0.0.1 as remote address

@dpratt
Copy link
Contributor Author

dpratt commented May 10, 2017

How do you have things configured? We are running it in production and it fixes the problem.

@zllovesuki
Copy link

zllovesuki commented May 10, 2017

@dpratt I compiled nginx-ingress-controller from the master branch, and run it via with hostNetwork: true:

apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  labels:
    k8s-app: nginx-ingress-controller
  name: nginx-ingress-controller
spec:
  replicas: 5
  selector:
    matchLabels:
      k8s-app: nginx-ingress-controller
  template:
    metadata:
      labels:
        k8s-app: nginx-ingress-controller
    spec:
      containers:
      - args:
        - /nginx-ingress-controller
        - --default-backend-service=$(POD_NAMESPACE)/default-http-backend
        - --configmap=$(POD_NAMESPACE)/nginx-conf
        env:
        - name: POD_NAME
          valueFrom:
            fieldRef:
              apiVersion: v1
              fieldPath: metadata.name
        - name: POD_NAMESPACE
          valueFrom:
            fieldRef:
              apiVersion: v1
              fieldPath: metadata.namespace
        image: zllovesuki/nginx-ingress-controller:master
        imagePullPolicy: IfNotPresent
        livenessProbe:
          failureThreshold: 3
          httpGet:
            path: /healthz
            port: 10254
            scheme: HTTP
          initialDelaySeconds: 10
          periodSeconds: 10
          successThreshold: 1
          timeoutSeconds: 1
        name: nginx-ingress-controller
        ports:
        - containerPort: 80
          hostPort: 80
          protocol: TCP
        - containerPort: 443
          hostPort: 443
          protocol: TCP
        - containerPort: 18080
          hostPort: 18080
          protocol: TCP
        readinessProbe:
          failureThreshold: 3
          httpGet:
            path: /healthz
            port: 10254
            scheme: HTTP
          periodSeconds: 10
          successThreshold: 1
          timeoutSeconds: 1        
        volumeMounts:
        - mountPath: /etc/nginx/template
          name: nginx-template-volume
          readOnly: true
      hostNetwork: true
      terminationGracePeriodSeconds: 30
      volumes:
      - configMap:
          defaultMode: 420
          items:
          - key: nginx.tmpl
            path: nginx.tmpl
          name: nginx-template
        name: nginx-template-volume

default/nginx-conf:

apiVersion: v1
data:
  enable-vts-status: "true"
  enabled-dynamic-tls-records: "true"
  error-log-level: warn
  keep-alive: "30"
  max-worker-connections: "10240"
  proxy-body-size: 2g
  proxy-read-timeout: "300"
  proxy-send-timeout: "300"
  ssl-ciphers: ECDHE-ECDSA-CHACHA20-POLY1305-D:ECDHE-RSA-CHACHA20-POLY130-D:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-RSA-AES256-SHA256:DHE-RSA-AES256-SHA:ECDHE-ECDSA-DES-CBC3-SHA:ECDHE-RSA-DES-CBC3-SHA:EDH-RSA-DES-CBC3-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA:DES-CBC3-SHA:!DSS
  ssl-dh-param: default/nginx-dhparam-4096
  worker-processes: "8"
kind: ConfigMap
metadata:
  name: nginx-conf
  namespace: default

nginx.tmpl is the same from your commit/PR except that I modify the logging to an external graylog server.

@dpratt
Copy link
Contributor Author

dpratt commented May 10, 2017

The important bit in the nginx template is in the server directive - make sure yours reads as follows, the part with proxy_protocol on port 442 is the important bit, if it's not configured the same, the remote address won't propagate properly.

    server {
     
       #content redacted
        {{/* Listen on 442 because port 443 is used in the TLS sni server */}}
        {{/* This listener must always have proxy_protocol enabled, because the SNI listener forwards on source IP info in it. */}}
        {{ if not (empty $server.SSLCertificate) }}listen 442 proxy_protocol{{ if eq $server.Hostname "_"}} default_server reuseport backlog={{ $backlogSize }}{{end}} ssl {{ if $cfg.UseHTTP2 }}http2{{ end }};

    }

Are you using the ingress controller to terminate SSL? If not, your problem is unrelated to this. Additionally, what nginx variable are you using to populate the remote address in your GELF message?

Are you guys piping all your outputs to graylog? If so, you might be interested in the config we use internally - we don't use the stock fluentd-elasticsearch k8s addon, but rather a custom DaemonSet that runs fluent and pipes all logs from all containers to Graylog.

To help with this, I've also modified our template to output access logs in JSON for better indexing - here's a relevant snippet.

    # filter numeric values so that we don't generate invalid json
    map $request_time $request_time_numeric {
      ~^(([0-9]*)|(([0-9]*)\.([0-9]*)))$ $request_time;
      default                            null;
    }

    map $request_length $request_length_numeric {
      ~^(([0-9]*)|(([0-9]*)\.([0-9]*)))$ $request_length;
      default                            null;
    }

    map $body_bytes_sent $body_bytes_sent_numeric {
      ~^(([0-9]*)|(([0-9]*)\.([0-9]*)))$ $body_bytes_sent;
      default                            null;
    }

    map $upstream_response_length $upstream_response_length_numeric {
      ~^(([0-9]*)|(([0-9]*)\.([0-9]*)))$ $upstream_response_length;
      default                            null;
    }

    map $upstream_response_time $upstream_response_time_numeric {
      ~^(([0-9]*)|(([0-9]*)\.([0-9]*)))$ $upstream_response_time;
      default                            null;
    }

    log_format json '{ '
         '"level": "info", '
         '"timestamp": "$time_iso8601", '
         '"message": "$status $request", '
         '"remote_addr": "{{ if $cfg.UseProxyProtocol }}$proxy_protocol_addr{{ else }}$remote_addr{{ end }}", '
         '"proxy_forwarded_for": "$http_x_forwarded_for", '
         '"remote_user": "$remote_user", '
         '"status": "$status", '
         '"request_method": "$request_method", '
         '"http_host": "$host", '
         '"http_protocol": "$pass_access_scheme", '
         '"request": "$request", '
         '"request_length": $request_length_numeric, '
         '"request_time": $request_time_numeric, '
         '"body_bytes_sent": $body_bytes_sent_numeric, '
         '"proxy_upstream_name": "$proxy_upstream_name", '
         '"upstream_addr": "$upstream_addr", '
         '"upstream_response_length": $upstream_response_length_numeric, '
         '"upstream_response_time": $upstream_response_time_numeric, '
         '"upstream_status": "$upstream_status", '
         '"http_referrer": "$http_referer", '
         '"http_user_agent": "$http_user_agent" '
    '}';

    {{/* map urls that should not appear in access.log */}}
    {{/* http://nginx.org/en/docs/http/ngx_http_log_module.html#access_log */}}
    map $request_uri $loggable {
        {{ range $reqUri := $cfg.SkipAccessLogURLs }}
        {{ $reqUri }} 0;{{ end }}
        default 1;
    }

    {{ if $cfg.DisableAccessLog }}
    access_log off;
    {{ else }}
    access_log /var/log/nginx/access.log json if=$loggable;
    {{ end }}
    error_log  /var/log/nginx/error.log {{ $cfg.ErrorLogLevel }};

@zllovesuki
Copy link

(Sorry I'm very sleepy I apologize if I'm being incoherent...)

sample from /etc/nginx/nginx.conf:

server {
    server_name echo.api.blue;
    listen 80;
    listen [::]:80;

    listen 442 proxy_protocol ssl http2;
    listen [::]:442 proxy_protocol  ssl http2;
    # PEM sha: 51318bfe15fceda4de78851efdb08d4e6f191aaf
    ssl_certificate                         /ingress-controller/ssl/default-echoserver-tls.pem;
    ssl_certificate_key                     /ingress-controller/ssl/default-echoserver-tls.pem;

    more_set_headers                        "Strict-Transport-Security: max-age=15724800; includeSubDomains;";

    vhost_traffic_status_filter_by_set_key $geoip_country_code country::$server_name;

    location /.well-known/acme-challenge {
        set $proxy_upstream_name "default-kube-lego-nginx-8080";
        port_in_redirect off;

        client_max_body_size                    "2g";

        proxy_set_header Host                   $best_http_host;

        # Pass the extracted client certificate to the backend

        # Pass Real IP
        proxy_set_header X-Real-IP              $remote_addr;

        # Allow websocket connections
        proxy_set_header                        Upgrade           $http_upgrade;
        proxy_set_header                        Connection        $connection_upgrade;

        proxy_set_header X-Forwarded-For        $proxy_add_x_forwarded_for;
        proxy_set_header X-Forwarded-Host       $best_http_host;
        proxy_set_header X-Forwarded-Port       $pass_port;
        proxy_set_header X-Forwarded-Proto      $pass_access_scheme;
        proxy_set_header X-Original-URI         $request_uri;
        proxy_set_header X-Scheme               $pass_access_scheme;

        # mitigate HTTPoxy Vulnerability
        # https://www.nginx.com/blog/mitigating-the-httpoxy-vulnerability-with-nginx/
        proxy_set_header Proxy                  "";

        # Custom headers

        proxy_connect_timeout                   5s;
        proxy_send_timeout                      300s;
        proxy_read_timeout                      300s;

        proxy_redirect                          off;
        proxy_buffering                         off;
        proxy_buffer_size                       "4k";
        proxy_buffers                           4 "4k";

        proxy_http_version                      1.1;

        proxy_cookie_domain                     off;
        proxy_cookie_path                       off;

        proxy_pass http://default-kube-lego-nginx-8080;
    }
    # enforce ssl on server side
    if ($pass_access_scheme = http) {
        return 301 https://$best_http_host$request_uri;
    }
    location / {
        set $proxy_upstream_name "default-echoserver-svc-9993";
        port_in_redirect off;

        client_max_body_size                    "2g";

        proxy_set_header Host                   $best_http_host;

        # Pass the extracted client certificate to the backend

        # Pass Real IP
        proxy_set_header X-Real-IP              $remote_addr;

        # Allow websocket connections
        proxy_set_header                        Upgrade           $http_upgrade;
        proxy_set_header                        Connection        $connection_upgrade;

        proxy_set_header X-Forwarded-For        $proxy_add_x_forwarded_for;
        proxy_set_header X-Forwarded-Host       $best_http_host;
        proxy_set_header X-Forwarded-Port       $pass_port;
        proxy_set_header X-Forwarded-Proto      $pass_access_scheme;
        proxy_set_header X-Original-URI         $request_uri;
        proxy_set_header X-Scheme               $pass_access_scheme;

        # mitigate HTTPoxy Vulnerability
        # https://www.nginx.com/blog/mitigating-the-httpoxy-vulnerability-with-nginx/
        proxy_set_header Proxy                  "";

        # Custom headers

        proxy_connect_timeout                   5s;
        proxy_send_timeout                      300s;
        proxy_read_timeout                      300s;

        proxy_redirect                          off;
        proxy_buffering                         off;
        proxy_buffer_size                       "4k";
        proxy_buffers                           4 "4k";

        proxy_http_version                      1.1;

        proxy_cookie_domain                     off;
        proxy_cookie_path                       off;

        proxy_pass http://default-echoserver-svc-9993;
    }

}

@dpratt
Copy link
Contributor Author

dpratt commented May 11, 2017

Looks okay to me - not sure why it's not working for you. The only moving part that hasn't been eliminated is the bits that produce your GELF message. To track this down, I'd suggest that you temporarily swap out GELF logging in your template for bone stock text nginx access logging and see what you get there.

@zllovesuki
Copy link

@dpratt Using your template still gives 127.0.0.1...

@zllovesuki
Copy link

OK, I found the issue... I'm using nginx directly on host network, and without proxy_protocol enabled to all (since there are hosts with HTTP only). Therefore, set_real_ip doesn't work...

Workaround would be:

# Pass Real IP

set $the_real_ip $remote_addr;
set $the_x_forwarded_for $proxy_add_x_forwarded_for;

if ($pass_access_scheme = 'https') {
    set $the_real_ip $proxy_protocol_addr;
    set $the_x_forwarded_for $proxy_protocol_addr;
}

proxy_set_header X-Real-IP              $the_real_ip;
proxy_set_header X-Forwarded-For        $the_x_forwarded_for;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants